ref(eap-outcomes): use sentry-options to change which timestamp to use#7838
ref(eap-outcomes): use sentry-options to change which timestamp to use#7838kenzoengineer merged 14 commits intomasterfrom
Conversation
MeredithAnya
force-pushed
the
kjiang/setup-sentry-options
branch
from
6c99601 to
77cf17e
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| @@ -1,3 +1,6 @@ | |||
| pub(crate) const SNUBA_SCHEMA: &str = | |||
| include_str!("../../sentry-options/schemas/snuba/schema.json"); | |||
There was a problem hiding this comment.
the schemas are now baked in during compile time.
this means during runtime, init_with_schemas should never fail when crate::SNUBA_SCHEMA is passed in (assuming the schema is not malformed, but this is caught in tests and our CI validation)
our only vulnerabilities now are options() and get()
| @@ -129,6 +129,13 @@ impl<TNext: ProcessingStrategy<AggregatedOutcomesBatch>> ProcessingStrategy<Kafk | |||
| for OutcomesAggregator<TNext> | |||
| { | |||
| fn poll(&mut self) -> Result<Option<CommitRequest>, StrategyError> { | |||
There was a problem hiding this comment.
@kenzoengineer so poll can be called like thousands of time a second i don't think we need to be checked every poll, I think we could add like a last_options_check time or something and check and refresh the value like every 5 seconds or 10 seconds?
There was a problem hiding this comment.
for posterity: this has been benchmarked and the normal options().get() call takes ~150ns, while a cached call with a 5 second TTL takes ~40ns
while theres a dramatic decrease, the numbers are still in the nanoseconds. We can keep it the naive way for now and in a future PR we can make this optimization if necessary
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| .ok() | ||
| .and_then(|o| o.get("consumer.use_item_timestamp").ok()) | ||
| .and_then(|v| v.as_bool()) | ||
| .unwrap_or(false); |
There was a problem hiding this comment.
Option read on every poll causes unnecessary I/O
Medium Severity
options("snuba").get("consumer.use_item_timestamp") is called on every poll() invocation. Since poll() can be called thousands of times per second, this causes excessive reads from the sentry-options backend (filesystem/ConfigMap). The sentry-options library's own quickstart example demonstrates reading values in a loop with a 3-second sleep, suggesting per-call reads are not meant for hot paths. A time-based cache (e.g., refreshing every 5–10 seconds) would maintain dynamic reloadability while avoiding the overhead, as the PR reviewer also suggested.
There was a problem hiding this comment.
this is incorrect. we only check memory.
4 participants
This is a duplicate of #7834, using sentry-options instead of the old config method.
#7836 should be merged first to ensure all the scaffolding works in production.
Because this is a duplicate, the logic, usage, and tests all mirror #7834. The only thing I've added is we use sentry-options instead.